Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP: Add new long_description_content_type kwarg #708

Closed
wants to merge 2 commits into from

Conversation

msabramo
Copy link
Contributor

@msabramo msabramo commented Jul 30, 2016

Work in progress

This is used to populate the newly proposed Description-Content-Type distribution metadata field, that is proposed in pypa/packaging.python.org#258.

We can discuss and evolve the implementation, but I don't think this should be merged until pypa/packaging.python.org#258 is accepted.

Related:

Branch

https://github.com/msabramo/setuptools/tree/long_description_content_type

Cc: @jaraco, @dstufft, @ncoghlan, @pfmoore, @nicktimko

This is used to populate the newly proposed `Description-Content-Type`
field, that is proposed in
pypa/packaging.python.org#258.
@msabramo msabramo force-pushed the long_description_content_type branch from b6fd44c to cdb4700 Compare July 30, 2016 15:48
@msabramo
Copy link
Contributor Author

I would like to add a test for this. Not expecting it to be difficult, but pointers to similar tests and such would be appreciated.

@jaraco
Copy link
Member

jaraco commented Jul 31, 2016

Given that the code is in dist.py, I would expect the test to be in test_dist.py, but I see no such test. It may be the case that you'll be writing the first tests for this. Of course, dist.py is a patch of distutils.dist, so maybe there are tests in the stdlib that apply.

It's an old package, so I'd say there's lots of cruft here - don't assume because you find it that it's recommended. Rely on pytest docs and your best instincts to put together the best tests you can. Thanks for all the work on this.

@xavfernandez
Copy link
Member

For Requires-Python and Metadata-Version I put the tests in https://github.com/pypa/setuptools/blob/d079163/setuptools/tests/test_egg_info.py#L213-L237

Which makes me think: is there a PEP defining this new field ? Should the Metadata-Version be bumped also ?

@msabramo
Copy link
Contributor Author

msabramo commented Jul 31, 2016

There is no PEP, but there is a PR to update the "Python Packaging Guide. @ncoghlan said on the distutils-sig mailing list that opening a a PR to PyPUG was the proper way now to propose new metadata fields as it's less of a barrier to entry than writing a PEP. I'm not sure if that's a precursor to creating a PEP or replaces it? Also I'm not sure whether adding one optional field requires the metadata version to be bumped or not. I'll let someone else chime in here on the proper procedure and I'll follow whatever it is.

Thanks for the test pointers!

@msabramo
Copy link
Contributor Author

msabramo commented Jul 31, 2016

Added a simple test in bd5403e, borrowing from the tests that @xavfernandez mentioned.

Test that specifying a `long_description_content_type` keyword arg to
the `setup` function results in writing a `Description-Content-Type`
line to the `PKG-INFO` file in the `<distribution>.egg-info` directory.

`Description-Content-Type` is described at
pypa/packaging.python.org#258
@ncoghlan
Copy link
Member

ncoghlan commented Aug 1, 2016

Yep, the PR-to-PyPUG is sufficient to add the new field (once accepted) - my view is that bumping the spec version mainly matters if we change the meaning of an existing field, since new fields are trivially detected by checking for them directly and having a defined fallback assumption to use if the field isn't found.

More generally, pypa/pypa.io#11 covers the status of the (currently stalled) efforts to both streamline and properly document the way we use the PEP process as part of PyPA interoperability discussions.

@msabramo msabramo force-pushed the long_description_content_type branch from f1343b3 to ebcdac8 Compare August 1, 2016 13:54
@msabramo
Copy link
Contributor Author

msabramo commented Aug 1, 2016

It doesn't look like the AppVeyor failure is related to my change, but not 100% sure.

@jaraco
Copy link
Member

jaraco commented Aug 1, 2016

Indeed - tests are now passing.

@msabramo
Copy link
Contributor Author

msabramo commented Aug 2, 2016

Cool, thanks @jaraco!

@jaraco
Copy link
Member

jaraco commented Dec 29, 2016

@msabramo Is this still a work in progress?

@msabramo
Copy link
Contributor Author

Possibly. I need to check the latest discussions on this idea which might be spread out on a few PRs and emails. And I'll be AFK for a few days so I won't have time to do it right away but it's on my list to do.

@nicktimko
Copy link

nicktimko commented Dec 29, 2016 via email

@msabramo
Copy link
Contributor Author

Yep. I need to check all the things and won't be able to for a bit.

If anyone is itching to help, they can send PRs to my branches for any changes folks suggested.

@jaraco
Copy link
Member

jaraco commented Jan 24, 2017

I'm going to close this to avoid re-reading it, though I gladly welcome anyone to revive the effort in a new PR.

@jaraco jaraco closed this Jan 24, 2017
@nicktimko
Copy link

Is there something wrong with this PR or just that it is dependent on a spec change that's still TBD? Just trying to improve your open ticket stats? 😉

@jaraco
Copy link
Member

jaraco commented Jan 24, 2017

Nothing specifically wrong with it, but it does appear to be in need of some attention. Instead, it's just periodically distracting me. I like to reserve open PRs for work that's being actively developed or ready for review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants